-
Notifications
You must be signed in to change notification settings - Fork 136
Ensure getCanBlaze is non nullable even when canBlaze is not set #14730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
🤖 Test Failure AnalysisYour tests failed. Claude has analyzed the failures - check the annotation for details. |
hichamboushaba
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @JorgeMucientes, but the change doesn't fix the issue, please check my comment below.
I've attempted migrating the field from Boolean to primitive value boolean as suggested in Linear issue, but it does require migration
Just confirming here, what error did you encounter after updating the type?
On my side, I've tried changing the type to primitive, and it didn't seem to need a migration. After the change, I faced a crash:
java.lang.NoSuchMethodError: No virtual method getCanBlaze()Ljava/lang/Boolean; in class Lorg/wordpress/android/fluxc/model/SiteModel; or its super classes (declaration of 'org.wordpress.android.fluxc.model.SiteModel' appears in /data/data/com.woocommerce.android.dev/code_cache/.overlay/base.apk/classes3.dex)
at com.woocommerce.android.ui.blaze.IsBlazeEnabled.invoke(IsBlazeEnabled.kt:25)
...
But this was probably just because of a bad cache, because after using ./gradlew :WooCommerce:assembleWasabiDebug --rerun-tasks, the issue was fixed, and I was able to run the app without issues.
I'm just sharing this to discuss, using the primitive type is cleaner, but if we are worried about any side effects (I tried forcing NULL value for the field, and it seems WellSql handled the change gracefully, but I didn't check enough to be confident), applying the fix on getCanBlaze will work too.
|
|
||
| public Boolean getCanBlaze() { | ||
| return mCanBlaze; | ||
| return mCanBlaze == true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not enough for the fix, we just moved the NPE exception here instead of IsBlazeEnabled.
When unboxing from the Boolean type to the primitive type, we'll get an NPE, and to reproduce it just comment the this.mCanBlaze = canBlaze; in the line 1117 to make sure we keep the default value (simulating what would happen if options is missing from /me/sites response), the app will crash with the following exception:
java.lang.NullPointerException: Attempt to invoke virtual method 'boolean java.lang.Boolean.booleanValue()' on a null object reference
at org.wordpress.android.fluxc.model.SiteModel.getCanBlaze(SiteModel.java:1113)
at com.woocommerce.android.ui.blaze.IsBlazeEnabled.invoke(IsBlazeEnabled.kt:25)
If we want to keep the Boolean class as type, we need to improve this check to something like:
| return mCanBlaze == true; | |
| return mCanBlaze != null && mCanBlaze; |
|
Another point @JorgeMucientes here, I think we might consider a beta fix with this given the number of crashes, WDYT? |
|
Thanks for reviewing this closely @hichamboushaba
Big 🤦🏼 . You are totally right. I wrongly assumed the unboxing would simply return the null value. But the unboxing was the root cause of the crash in the first place.
The exact same crash you shared. And I wrongly concluded it was due to the type change instead of checking that the actual reason was outdated generated function signatures. I actually ran a
Yes, I agree it's cleaner, and I think we should aim for that solution. Looks safe enough, worst case scenario, if we missed some corner case for users that currently have
Yeah, good point, given this started happening couple days ago in the latest release its probably worth fixing in beta. I'll move the changes to the release branch. |
|
Closing in favor of applying the fix to |
Yes, that's exactly how I tested it too. |
Fixes WOOMOB-1442
Description
I couldn't reproduce this crash, but the fix ensures
mCanBlazeis never null.I've attempted migrating the field from
Booleanto primitive valuebooleanas suggested in Linear issue, but it does require migration. So the simplest approach was to handle nullability atgetCanBlaze()function level.Steps to reproduce
Couldn't reproduce this either.
Testing information